Codecov Report

Merging #31 (600bcb0) into develop (600bcb0) will not change coverage.
The diff coverage is n/a.

❗ Current head 600bcb0 differs from pull request most recent head 324c7ea. Consider uploading reports for the commit 324c7ea to get more accurate results

@@           Coverage Diff            @@
##           develop      #31   +/-   ##
========================================
  Coverage    93.48%   93.48%           
========================================
  Files            9        9           
  Lines          583      583           
========================================
  Hits           545      545           
  Misses          38       38           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 600bcb0...324c7ea. Read the comment docs.

The first commit of this PR (7f9a0ba) should be fine, but it failed the code-coverage check. After deeper investigation, I don't see why lines 186 + 187 of the (modified) 'object_pool.hpp' are marked as yellow/partial coverage. The unit tests should already be properly covering both the case where the constructor throws, and the case where it does not.

For the second commit (311661f), I decided to try adding a simple unit-test where the UserAllocator "malloc" would fail (return zero), just to increase the code coverage. However, that fails the valgrind build/test for some reason that doesn't make sense to me. The error-message indicates that the 'cdtor_checker::check_out()' is failing, but in the lines that I added, I don't see where the "tester" object is ever being constructed. The always-fails "malloc()" should be preventing the placement-new from ever being run, so I'm not sure where the "tester" object being destructed is coming from.

I'm at just about the limit of what I can reasonably test here, as I am doing my development on Windows and don't have local access to valgrind. I'm also relatively new to github, so I'm not sure whether it is easy for you to pick up the "first half" of this PR, or whether it would be easier if I did something differently on my end. Please advise.

@jbherdman please rebase on develop for a more complete CI suite

Hi I just wanted to check if this pull request is going to get merged or - if not - I could somehow help getting it merged? For my projects it would be incredibly useful.

Sorry for the delay. Rebased to the latest develop, and it looks like it passes the CI checks now.

If there is anything more that needs to be done on my end, just let me know.

@jeking3 From your point of view, is there anything else to do? I would love to see this PR get merged.

I appreciate that maintaining open source is really hard work and I don't want to be annoying. If there's something left to do, I'd be happy to do it.

Thanks for rebasing; looks like we have some issues to deal with identified by CI. There are some untested branches as well as a valgrind error (invalid write) which was not there before.

@jeking3 Looking at the logs, the valgrind error (invalid write) is only triggered by the pre-existing test_valgrind_fail_2.cpp.

That appears to be an intentional failure-case (intentional write to released memory). I don't know why CI did not catch that issue before, but I see no reason why that would be a new failure under this PR.

@jeking3 I believe the valgrind error (invalid write) is a red herring. Reading through the logs, I only see those lines as appearing with a "(failed-as-expected)" line afterwards.

The valgrind failure might be coming from this line:
test 'objs.find(This) != objs.end()' failed in function 'void cdtor_checker::check_out(void *const)'

However, the only way I can see that failing would be if the destructor is being called with this == NULL. I have added an extra assert to try to highlight that case/issue in CI, as I have no local mechanism to run valgrind.

I have also added additional coverage-tests for the case where the variadic-template constructor fails by throwing an exception.

@jeking3 I found and fixed the problem that was causing the valgrind issue.

The special BOOST_POOL_VALGRIND version of the code was not behaving correctly when faced with a user-allocator that returned NULL (as added to enhance coverage testing). It was adding the NULL to its internal list of allocated-blocks, leading to the issues later on with invalid destructor calls, etc.

As for the Codecov report, it seems to only be looking at the first commit in this PR? I'm not sure how to fix that, shy of, I suppose: deleting my fork, re-forking, making all the changes in a single commit, and then opening a new PR? There must be an easier way, but I don't use git/github very often.

@fschoenberger Maybe a dumb idea on my part, but did you want to make your own fork, include my changes (all in a single commit), and submit it as new PR? That should get proper code-coverage numbers, without me needing to poke around and figure out how to 'squash' the commits on my end. I seem to have done something wrong with mixing rebases + merges, and GitHub Desktop is just not letting me do what I want to do. (I don't care about getting "credit" or whatever for the changes, I just want the enhancements to make their way into Boost.)

@fschoenberger Maybe a dumb idea on my part, but did you want to make your own fork, include my changes (all in a single commit), and submit it as new PR? That should get proper code-coverage numbers, without me needing to poke around and figure out how to 'squash' the commits on my end. [...]

@jbherdman Sure, I have create a new Pull Request.